GH-48408: [C++] Enable ULP-based float comparison#49290
GH-48408: [C++] Enable ULP-based float comparison#49290andishgar wants to merge 2 commits intoapache:mainfrom
Conversation
|
|
andishgar
force-pushed
the
enable_ulp_based_comparison
branch
from
c79f91f to
9cd0147
Compare
|
@pitrou , I would appreciate it if you could review this. |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR @andishgar . As you'll see in the comments below, I think it would be nice to come up with a nicer API for EqualOptions.
| bool use_atol_ = false; | ||
| bool use_schema_ = true; | ||
| bool use_metadata_ = false; | ||
| bool use_ulp_distance_ = false; |
There was a problem hiding this comment.
We could have a enum to simplify those flags slightly, for example:
enum FloatComparison { Exact, Atol, Ulps };
FloatComparison float_comparison_ = Exact;Alternatively, we could use a std::variant to also encompass the numeric parameters:
struct ExactComparison {};
struct UlpComparison { int max_ulps; }
struct AtolComparison { double atol; }
// Defaults to ExactComparison
std::variant<ExactComparison, UlpComparison, AtolComparison> float_comparison_ = {};| auto res = EqualOptions(*this); | ||
| res.ulp_distance_ = v; | ||
| return res; | ||
| } |
There was a problem hiding this comment.
This is starting to be a lot of methods just to customize FP comparison. Also, usually you know the number of ULPs you want, so you have to call two methods (use_ulp_distance and ulp_distance) which feels pointlessly complicated.
I wonder if we could have a single method to encompass all these usages, for example:
auto options = EqualOptions().float_comparison(EqualOptions::Atol(1e-5));
auto options2 = EqualOptions().float_comparison(EqualOptions::Ulps(2));
github-actions
bot
added
awaiting review
2 participants
Rationale for this change
Enable ULP-based floating-point comparison.
What changes are included in this PR?
Add
arrow::EqualOptions::use_ulp_distanceandarrow::EqualOptions::ulp_distance.Are these changes tested?
Yes.
Are there any user-facing changes?
Yes. The ULP-based comparison method is enabled via
arrow::EqualOptions::use_ulp_distanceandarrow::EqualOptions::ulp_distance.